[feat] Sign-in parity, chat-package re-sync, and the mobile UX pass (12/12) - #5691
[feat] Sign-in parity, chat-package re-sync, and the mobile UX pass (12/12)#5691ardaerzin wants to merge 26 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis change expands the mobile application with authentication, context and session navigation, chat transcript replay, approval steering, shared UI components, OAuth callback routing, and updated mobile design and execution documentation. ChangesMobile planning and contracts
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant SignInScreen
participant MobileAuth
participant OAuthProvider
participant AuthCallback
participant MobileGate
SignInScreen->>MobileAuth: Start password, OTP, or OIDC sign-in
MobileAuth->>OAuthProvider: Redirect for OIDC authorization
OAuthProvider->>AuthCallback: Return callback parameters
AuthCallback->>MobileAuth: Complete OIDC sign-in
MobileAuth->>SignInScreen: Report authentication outcome
MobileGate->>AuthCallback: Pass callback route without mobile redirect
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
28314ae to
cc14c6a
Compare
a0ceb44 to
553ef34
Compare
cc14c6a to
0aa2522
Compare
553ef34 to
61f34b3
Compare
0aa2522 to
cfb9c84
Compare
61f34b3 to
ccdc697
Compare
cfb9c84 to
defca8b
Compare
ccdc697 to
5c01194
Compare
defca8b to
de37daa
Compare
5c01194 to
a60bf4e
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
de37daa to
94e03ca
Compare
a60bf4e to
59641db
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
94e03ca to
52d6e1f
Compare
59641db to
4ed04bf
Compare
52d6e1f to
4c08deb
Compare
The approval dock gains a flag-gated Redirect control that sends the denial and a short instruction in one respond call. Delivery is cold-replay-only: a warm-parked resume answers the harness gate on the still-pending original prompt and never prompts again, so the note is dropped (measured live; the model then re-tried the denied work with a sibling tool and raised a new gate). The harness has no reject-with-feedback channel either - PermissionReply is once|always|reject - so the control stays hidden unless NEXT_PUBLIC_AGENT_CHAT_STEER=true, matching the desktop flag.
Providers only ever redirect to the one URI registered for them, the desktop `/auth/callback/<providerId>`, so a mobile SSO flow could never land in /m and the mobile sign-in had to tell users to switch to the desktop app. The mobile app now marks the flow with a short-lived cookie before leaving for the provider. The desktop forward gate reads it and forwards the landing to `/m/auth/callback/...` with the query intact, where the OAuth state (same-origin sessionStorage) is still readable — no new redirect URI has to be registered with any provider. `?view=desktop` still wins, and the other desktop-only exceptions are untouched. The reverse gate also stops bouncing any `/auth/callback` off /m: an OAuth landing must complete where it lands or the one-time code is lost.
/m/auth only did email+password and told everyone else to use the desktop app. It now offers every method the deployment enables, derived from the same env flags as the desktop's getEffectiveAuthConfig: - Passwordless: request a code, verify it, resend on a 60s cooldown, with each SuperTokens status mapped to a retry-in-place or a restart. - Social: one button per configured OAuth client id, in the desktop's order, running the redirect flow through the shared device gate's mobile callback. - Organization SSO: /auth/discover by work email, then the same redirect flow (404 on OSS reads as "no SSO", not an error). - Email/password unchanged; an SSO-only deployment no longer shows an email form, and a deployment with nothing enabled says so instead of rendering an empty card. Every route lands the same way it already did — invalidate the cached ["mobile","projects"] verdict, then hand over to the root context resolver. The pure halves (provider derivation, mode selection, the OTP state machine, discovery parsing) are unit-tested; 40 -> 83 tests.
…calStorage key
The package copy of the agent-chat message store writes the same localStorage key
as the desktop original but had drifted twice: it used jotai's default storage,
re-enabling the cross-tab `storage` subscribe the original deliberately strips
(an incoming replacement unmounts a streaming conversation and orphans its stream
mid-turn), and its quota guard returned void, so there was no record-count
watermark counterpart to `agenta:agent-chat:record-counts`.
Port `tabLocalStorage()`, the watermark store and its read atom, the
`{evicted, persisted}` quota-guard result, `persistSessionMessagesAtom`'s
`recordCount`, and the joint deletion path (exposed as a write atom here, since
the scope-keyed delete/close/prune callers stay in the app layer).
…eam nest The badge read `flags.is_running` raw, so it disagreed with the desktop tab dot on every row the coarse filter returns but the flags contradict: a flagless or proc-dead (zombie) row was badged live, where the desktop dot reads it as idle. Split on `deriveStreamNest` instead — running wins, then alive, everything else is omitted — so both surfaces answer "running"/"alive" from one derivation. Polling cadence and the react-query shape are untouched.
…termark The chat screen hand-rolled its adoption logic on message counts alone, so it missed the case the shared guard exists for: a turn that grows in place — tool results landing, an approval round-trip completing — keeps its message count, and mobile kept rendering the partial copy (issue #5530). Mobile now calls `shouldAdoptServerTranscript`. Its inputs map as: `busy` is always false (mobile never holds a live stream), and the watermark is an in-memory ref rather than desktop's persisted record-counts store, since mobile caches no transcript. The mapping lives in `transcriptAdoption.ts` so it is testable without mounting the hook. The trailing-refresh queue, the cancellation flag and the loading/ready/empty transitions are unchanged. The guard needs a record count, so the package's `loadSessionMessages` regains the `SessionTranscript` return its OSS original already carries (the copy is byte-identical again below its header); `useAgentConversation` follows the new shape and now files the record count with the transcripts it persists.
… switchable Sign-in and the OIDC callback carry the Agenta wordmark, inlined rather than served from public/ (a bare <img src="/assets/..."> misses the /m basePath) and drawn in currentColor so one component covers both themes. Header pinning becomes one ScreenScaffold instead of a shape hand-copied per screen: a h-dvh column whose middle is the only scroller. The project picker never had it and scrolled its own title away. The chat dock is passed as a footer only when it renders, so the scaffold's safe-area rule holds instead of silently lapsing when no gate is pending. The workspace line is now a real selector: chips when there are several, a labelled name when there is one (the default workspace is called "Default", indistinguishable from a project of that name). Reaching the picker at all needed ?switch=1, since a stored context auto-forwards past it — and that decision is gated on router.isReady, without which the first render forwards before the switch intent is knowable. Signed-out copy no longer claims you signed out on a screen that also renders for someone who never signed in.
123 commits in one PR is not reviewable. Measured against candidate lanes, only three files are touched by two lanes, and in each the lanes stack in the order the edits happened — so a chronological path-partitioned replay needs no hunk surgery. Twelve lanes, with the empty diff against the branch tip as the correctness gate.
The replay adapter emits `file` parts for a turn that carried attachments (see the package lane). The mobile transcript had no renderer for them, so those parts were parsed and then dropped on the floor. `AttachmentPart` draws them; `TurnRow` routes `file` parts to it.
Path-partitioned replay was rejected during execution: a pre-flight check over the real commit x file matrix found 42 files touched by more than one lane, 22 with an edit order no lane ordering can respect — each of which would have had a lower lane replay a newer version underneath a higher one, silently dropping edits. Contiguous ranges of the linear history cannot have that failure, and need no replay at all. Records the 12 lanes as built, and that fixed merge order is the trade.
The list never refreshed. Only staleTime was set, no refetchInterval, and the app disables refetchOnWindowFocus globally — so a session created on desktop stayed invisible until the screen remounted. The badges on existing rows polled while the set of rows was frozen. Poll the newest page as a SEPARATE query rather than adding refetchInterval to the infinite one: refetching an infinite query refetches every page scrolled into, so its cost grows with scroll depth. This stays one request per tick. Foreground-only, plus a visibilitychange refetch so returning from the lock screen does not wait out the interval. The merge dedupes on session identity and lets the head win on order: a session that just became active is returned at its new position and would otherwise render twice — fresh at the top, stale in the page it was fetched in.
Statuses rendered as bare coloured words butted against a title, so "new session test running" read as a sentence rather than a name plus its state. One StatusTag pill with its own border and background now carries all of them: the row badges (running/live/approval/ ended), the chat running bar, the transcript's awaiting-approval marker, and the list header's pending count. A dot marks the two tones that mean something is happening, so the distinction does not rest on colour alone. The title truncates independently of its tags, which the old inline spans could not do — a long name pushed them off the row.
The live and muted tones had byte-identical class strings, so a warm session and an ended one differed only by a dot — which is why live read as inert grey. They are opposite states. Bridge the palette semantic success role into the mobile theme (it already carries error) and give live its own tone. The three states are now separable without reading the label: accent means act on this, success means healthy and warm, muted means nothing is happening.
"2 approvals pending" told you work existed and left you to find it. The count is now a toggle that narrows the list to the sessions waiting on you. It counts SESSIONS, not interactions — the filter shows rows, and one session can hold several gates, so the old number could exceed the rows it produced. The interactions poll is project-wide while the list is paged, so a waiting session can sit beyond what has been fetched. Rather than silently showing a short list, the filter reports how many are further down and offers to load them; the empty case says the same instead of claiming there is nothing to do. Answering the last gate clears the filter rather than stranding the user on an empty list.
…hrase "show only these" pointed at nothing — while the filter is off the rows it means are not on screen, and a pill next to an underlined phrase reads as two controls when it is one. One toggleable chip carries it: the icon supplies the affordance (funnel to apply, cross to clear) so the visible label stays the count, and the spoken label is the full sentence the visible text should not have to be. The unloaded-count copy loses its instruction too — the Load more button is directly beneath it.
Three stacked full-width rows plus 44px tap targets on text-sized controls left roughly 150px of chrome above the first session — most of a phone screen's useful top half. Search and the filter now share a row, since both narrow the same list. The chip and the project switcher keep their 44px touch targets through an `after` pseudo-element instead of their own height, so a 32px chip and an 18px switcher row still hit the minimum. Header is about 84px. The search input keeps text-base: below 16px iOS zooms the viewport on focus.
The workspace governs everything below it, but it sat inside the scrolling list — with 50 projects it scrolled away, leaving no indication of which workspace the rows belonged to and no way to change it without scrolling back. It moves into the pinned header beside the title: chips when there are several, a labelled name when there is one. The list below becomes the selected workspace's projects, which also drops the nested section-per-workspace shape it no longer needs. Splits the old component in three along the way — the selector, the list, and the grouping, which was an inline useMemo and is now a covered pure function.
Review of the plan set found the documents lagging the code in about twenty places. Each correction was checked against the tree, not against the plan: - WP0's residual gaps are all implemented. The section is re-headed as the analysis that produced the work, and the step text now carries the two things review changed during execution: ordering rides `coalesce(updated_at, created_at)` because `updated_at` is nullable and a DESC sort puts NULLs first, and the references hydration requires the batch `latest_turn_per_session` helper rather than the per-session one, which would make /sessions/query an N+1 path. - The infra-tail plan's snippets now match the workflow: dispatch inputs through `env:`, exact status assertions in the smoke checks, the segment-exact Traefik rule, `pipefail` before the piped verifications, and a bounded readiness poll with a cleanup trap instead of `sleep 3`. - The WP1 plan gains the repo-wide `cd web && pnpm lint-fix` step, the corrected Turbo inputs, package-relative paths in the shadcn verification (it runs after `cd web/mobile`), and the dark role-map values review actually landed. - `NoMobilePageWrapper` retires when the gate is turned on, not when its code ships. The design doc said the latter, which would leave a mobile visitor with neither the gate nor the blocker. - The m3 plan states the three event types the endpoint emits, records that the channel's project comes from the credential and never the URL, corrects the claim that a session-scoped stream can retire the list's project-wide badge polls, and states the unbounded stream age as a known gap with its fix rather than as an aside. - The approvals plan is re-statused as a pre-execution snapshot, since M2 shipped a different client payload than it sketches. - The chat contract's `useApprovalDock` line named an argument the hook does not take and a `renderer` it does not return, while omitting `open` and `responding`. - README: the WP5 status contradiction, the doubled path in the standalone check, a completed `.gitignore` chore still listed as pending, the commit count against the review record, and `run.sh` invocations that named neither the entrypoint nor the edition.
…e flag The handback sat below `if (!input.gateEnabled) return`. The gate ships DEFAULT OFF, so in the configuration everyone actually runs, a callback the mobile app started stayed on the desktop route — where the OAuth state it needs does not exist, because that state lives in /m's same-origin sessionStorage. Mobile SSO could not complete at all. The cookie is an explicit intent set by /m moments earlier, not a device heuristic, so it is not something the device gate should be able to switch off. It now runs first. `?view=desktop` still wins: it is the user saying "keep me here", and moving the callback check earlier would otherwise have let the handback beat the escape hatch. The existing precedence test caught exactly that.
/auth/discover was hand-cast from unknown through three layers of optional chaining. A backend shape change would degrade silently into "no SSO options", which a user reads as "my org has no SSO" rather than as a failure. It now validates with zod through safeParseWithLogging, like the projects fetch. Not moved onto the Fern client, which the review also asked for: /auth/discover is EE-only and is not in the generated client, so there is no resource accessor to call. That needs codegen first, and would then let this drop the hand-rolled request as well. Deliberately does NOT restore the "View desktop site" footer that a review asked for. The design puts that affordance in the project drawer (hamburger sheet: workspace/project switcher, user info, sign-out, view desktop site), not on individual screens, and that drawer is still to be built. Landing it as a page footer now would mean removing it again later. The gate ships default-off, so nothing is trapped meanwhile, but the drawer has to exist before AGENTA_MOBILE_GATE is turned on.
…lears The resume settled on the pending-gate COUNT reaching zero. Answering the last gate of a turn routinely raises the next one in the same poll, so the count reads one before and one after, the phase stays `resuming`, and every approval button is disabled until the 60-second failure-path timeout. From the phone that looks like the app hung on the tap. The hook now remembers which approval ids its submit actually answered and settles once those ids are gone from the transcript, whatever else is pending. `hasSettledResume` holds that rule as a pure function in `approvalTargets.ts`, next to the other selection logic and covered by the sibling tests, including the replaced-by-a-new-gate case that count-based settling misses. `ChatScreen` passes the pending ids instead of the count; the count is still what drives the poll interval.
… a way out Two dead ends on the sessions list. `querySessions` resolves `null` instead of throwing, so a failed later page was appended to `data.pages`. `getNextPageParam(null)` then returns undefined, `hasNextPage` goes false, and the retry button rendered by `laterPageFailed` called `fetchNextPage()` into a short circuit: no request, no change, nothing to tell the user why. The page now rejects, which leaves the last GOOD page as the cursor source, so the retry asks for the same page again. `classifyPageFailure` reads that shape: a rejected page never lands in `pages`, so an error alongside rows already on screen is the later-page case, not a whole-list failure. It still handles `null` entries for any caller that resolves them. The whole-list error state offered only Retry. The screen has already dropped the stored context by then, but that only helps the NEXT visit to `/m` -- the user stays parked on a route for a project that may be deleted or no longer theirs, with the picker and the signed-out notice both out of reach. It now also links to the picker. Retry stays first: the common cause is transient.
…ludes The interactions poll is project-wide; the list is not. With a search active, `merged` holds only matching sessions, so nearly every waiting session the poll knows about counted as "further down the list, not loaded yet" -- and offered a Load more that could never surface them, because paging the searched query only ever returns more matches. Under a search there is no unloaded count to report, so `filterPendingRows` reports none, and the empty state says the waiting sessions do not match rather than that there are none. Without a search nothing changes: the count is what makes "2 waiting" next to one row explainable, and it stays.
Every screen carried a generic title, so a phone with several tabs open showed "Sessions",
"Session" and "Agenta Mobile" with nothing to tell them apart, and the browser fell back to the
raw URL wherever a shell had none.
Titles now come from the screens, which already hold the data:
picker Choose a project - Agenta
sessions Sessions - <project> - Agenta
chat <session name> - Agenta
auth Sign in - Agenta
Most specific part first, because a phone's tab strip and app switcher show only the leading
characters. PageTitle drops empty parts, so a screen can pass a name that has not resolved yet
without rendering "undefined", and the page shells stay thin route shells.
The project name comes from the picker's own query key and staleTime, so there is no extra
request; ProjectSwitcher was reading the same thing through its own copy of that query, and both
now share useCurrentProject.
0da367b to
5e00f40
Compare
fa9fe83 to
e6b4ff5
Compare
A tool row rendered as one line of text: name, state, and the summary inline. The summary is the
tool's raw output, so a read that returns a page of YAML pushed the assistant's actual reply far
down the screen, and the state string ("output-available") was noise a reader never needs.
It is now a details/summary row like the Thoughts block above it: a small state dot plus the tool
name, with the output behind a tap. The dot turns destructive on output-error, which is the one
state worth seeing without opening the row.
Context
The top of the stack: sign-in parity, the first consolidation work against the copy-drift the stack created, and the mobile UX pass that came out of using it on a real phone.
Changes
Sign-in parity.
/mgains email OTP, social providers, and org SSO. Providers only ever redirect to the one registered URI (the desktop/auth/callback/<id>), so the desktop hands a mobile-initiated callback back to/mrather than requiring a second registration.Consolidation, wave 0.
@agenta/chat's message store had drifted from the OSS original it was copied from on the same localStorage key: it had re-enabled the cross-tab sync OSS deliberately strips, and dropped the record-count watermark. The replay adapter had drifted too, missing the approval-resume handling (a resumed turn replayed as still parked, so a reload kept the approval dock up) and user attachments (a message with files replayed as bare text). Both re-synced, with the mobile side rendering attachments.Correctness. A replayed approval envelope now names its tool. Without it the runner's resume nudge said "Call the tool again", naming nothing the model could call, and the model narrated a fabricated execution instead of re-issuing the call.
UX pass. Statuses became real tags instead of bare coloured words (and
liveno longer renders identically toended). The pending count became a filter rather than a statistic. The session list polls its newest page, so a session created on desktop appears without a manual refresh. The header stopped eating the first screen, and the workspace selector moved into it.Tests / notes
@agenta/chatsuites both grew with this work; the settle fix is covered by tests ported from the OSS original, and I verified they fail without it.What to QA
/mwith Google, with an email OTP, and with org SSO. Each completes and lands you in the app.